Implement partial_sort_unstable for slice #149318

tisonkun · 2025-11-25T15:36:46Z

This refers to #149046.

rustbot · 2025-11-25T15:36:51Z

rustbot has assigned @scottmcm.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

library/core/src/slice/mod.rs

Signed-off-by: tison <wander4096@gmail.com>

tisonkun · 2025-11-25T17:58:23Z

cc @orlp

orlp

Left some remarks, some on style, but also some with substance.

Besides comments on the code that's written, I do note a lack of tests?

View changes since this review

orlp · 2025-11-25T19:56:32Z

library/core/src/slice/mod.rs

+    /// as if the whole slice were sorted in ascending order.
+    ///
+    /// This sort is unstable (i.e., may reorder equal elements), in-place (i.e., does not
+    /// allocate), and *O*(*n* + *k* \* log(*k*)) worst-case.


It's not specified what k is.

orlp · 2025-11-25T19:58:35Z

library/core/src/slice/mod.rs

+    ///
+    /// If the implementation of [`Ord`] for `T` does not implement a [total order], the function
+    /// may panic; even if the function exits normally, the resulting order of elements in the slice
+    /// is unspecified. See also the note on panicking below.


I don't think we need to repeat the entire comment with examples from sort_unstable_by here, I think we can just refer the reader to its comment. This concerns this paragraph as well as all following paragraphs up until # Panics.

orlp · 2025-11-25T20:02:14Z

library/core/src/slice/mod.rs

+    where
+        F: FnMut(&T, &T) -> Ordering,
+        R: RangeBounds<usize>,
+    {


I think this implementation should be under sort::unstable::partial_sort instead, and like its cousin sort::unstable::sort take a function returning a boolean. Then partial_sort_unstable(_by(_key)) can all dispatch to that one directly.

Then partial_sort_unstable(_by(_key)) can all dispatch to that one directly.

Not quite. I think sort_unstable and sort_unstable_by_key should instead redirect to sort_unstable_by and so does select_nth_unstable series, as what binary_search and partition_dedup do.

The final implement may be factored out to sort::unstable::partial_sort. But I'd prefer to align all these styles in a follow-up patch.

And I think that would be a mistake, and would reject that follow-up patch. The sorts are incredibly sensitive to inlining and I would highly prefer to minimize the number of steps of indirection to maximize the chance the comparison is inlined.

Fair enough. Let me try to find a place for the common partial_sort impls :D

orlp · 2025-11-25T20:07:35Z

library/core/src/slice/mod.rs

+    /// Partially sorts the slice in ascending order **without** preserving the initial order of equal elements.
+    ///
+    /// Upon completion, the elements in the specified `range` will be the elements of the slice
+    /// as if the whole slice were sorted in ascending order.


This is missing the guarantees I mentioned in my comment on the ACP about the elements that come before and after the sorted range.

orlp · 2025-11-25T20:08:34Z

library/core/src/slice/mod.rs

+        let Range { start, end } = slice::range(range, ..len);
+
+        if start + 1 > end {
+            // target range is empty, nothing to do


This is incorrect if we do provide the guarantees for elements before and after the range I mentioned in my comment on the ACP. I think those guarantees are more useful than allowing the partial sort by an empty range in the middle to be a no-op.

Great! Let me try to figure out how to work it out. From what we have now, perhaps we'd select a on a..a for partitioning, and properly handle the case where the slice is empty to avoid panic.

orlp · 2025-11-25T20:09:26Z

library/core/src/slice/mod.rs

+        let len = self.len();
+        let Range { start, end } = slice::range(range, ..len);
+
+        if start + 1 > end {


What a very strange way to write start == end. slice::range already disallows inverted ranges.

Fair enough. I'd leave it for now and it's OK to change to start == end if it's more preferred. This is mainly personal preference.

Well it also costs two extra instructions compared to start == end: https://rust.godbolt.org/z/fe37Y8Pb3.

Thanks for your information! I'll update it in my next commit then.

orlp · 2025-11-25T20:12:44Z

library/core/src/slice/mod.rs

+            return;
+        }
+
+        let index = start;


I think assigning start to a variable which is used only once and is later shadowed with a different definition is not a good idea. Just directly pass start to partition_at_index IMO.

Make sense.

orlp · 2025-11-25T20:15:40Z

library/core/src/slice/mod.rs

+        let (_, _, rest) =
+            sort::select::partition_at_index(self, index, |a, b| compare(a, b) == Less);
+
+        if start + 2 > end {


Again, very strange way to write end - start == 1. I think this branch is worthwhile though, select_nth_unstable does guarantee that this singular element is in its correct place.

Despite the style issue, I'm considering also what we'd specially handle whenstart == 0 so that a partitioning by end - 1 + sort can eliminate the first select_nth(0). But I'm not sure if the complexity handling more cases worth.

start == 0 should be the most common case. And a..a would be easier to handle if we distinguish 0..0 (nothing to do) and a..a (a > 0) (must have (a-1)-th element to be partitioned on).

Let me try to apply this idea.

tisonkun · 2025-11-26T02:14:12Z

I do note a lack of tests?

Doc tests cover most branches. I don't find a dedicated file to cover its cousin sort_unstable. If you can point me to one, I'm glad to add cases there.

orlp · 2025-11-26T07:29:05Z

The examples can change at any time. And you didn't test, for example, the post-condition that all elements ..start are less than or equal to the elements start..end and that those are less than or equal to the elements end.., including for the zero-length case.

tisonkun · 2025-11-26T07:37:28Z

The examples can change at any time. And you didn't test, for example, the post-condition that all elements ..start are less than or equal to the elements start..end and that those are less than or equal to the elements end.., including for the zero-length case.

Thanks and yes. Do you know where the unit tests of sort/sort_unstable locate?

orlp · 2025-11-26T07:41:01Z

I believe the bulk is found in https://github.com/rust-lang/rust/blob/main/library/alloctests/tests/sort/tests.rs.

orlp · 2025-11-26T09:39:57Z

What I suggested in the ACP was a sketch implementation, I did some more thinking and I think the following handles all corner cases nicely:

pub fn partial_sort<T, F, R>(mut v: &mut [T], range: R, is_less: &mut F)
where
    F: FnMut(&T, &T) -> bool,
    R: RangeBounds<usize>,
{
    let len = v.len();
    let Range { start, end } = slice::range(range, ..len);
    
    if end - start <= 1 {
        // Can be resolved in at most a single partition_at_index call, without
        // further sorting. Do nothing if it is an empty range at start or end.
        if start != len && end != 0 {
            sort::select::partition_at_index(v, start, is_less);
        }
        return;
    }
    
    // Don't bother reducing the slice to sort if it eliminates fewer than 8 elements.
    if end + 8 <= len {
        v = sort::select::partition_at_index(v, end - 1, is_less).0;
    }
    if start >= 8 {
        v = sort::select::partition_at_index(v, start, is_less).2;
    }
    sort::unstable::sort(v, is_less);
}

And to formalize the post-conditions, I think the following should hold after a call to v.partial_sort_unstable(b..e):

for i in 0..b {
    for j in b..n {
        assert!(v[i] <= v[j]);
    }
}
for i in 0..e {
    for j in e..n {
        assert!(v[i] <= v[j]);
    }
}
for i in b..e {
    for j in i..e {
        assert!(v[i] <= v[j]);
    }
}

rustbot assigned scottmcm Nov 25, 2025

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Nov 25, 2025

tisonkun commented Nov 25, 2025

View reviewed changes

library/core/src/slice/mod.rs Outdated Show resolved Hide resolved

This comment has been minimized.

Sign in to view

tisonkun force-pushed the slice_partial_sort_unstable branch from 7af04ad to 115ac5c Compare November 25, 2025 16:13

tisonkun mentioned this pull request Nov 25, 2025

Tracking Issue for adding partial_sort_unstable to [T] #149046

Open

4 tasks

This comment has been minimized.

Sign in to view

Implement partial_sort_unstable for slice

0e87d5d

Signed-off-by: tison <wander4096@gmail.com>

tisonkun force-pushed the slice_partial_sort_unstable branch from 115ac5c to 0e87d5d Compare November 25, 2025 16:42

Better document for partial_sort_unstable

f9a09e0

Signed-off-by: tison <wander4096@gmail.com>

orlp suggested changes Nov 25, 2025

View reviewed changes

Implement partial_sort_unstable for slice #149318

Are you sure you want to change the base?

Implement partial_sort_unstable for slice #149318

Conversation

tisonkun commented Nov 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rustbot commented Nov 25, 2025

Uh oh!

Uh oh!

This comment has been minimized.

This comment has been minimized.

tisonkun commented Nov 25, 2025

Uh oh!

orlp left a comment • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tisonkun Nov 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tisonkun commented Nov 26, 2025

Uh oh!

orlp commented Nov 26, 2025

Uh oh!

tisonkun commented Nov 26, 2025

Uh oh!

orlp commented Nov 26, 2025

Uh oh!

orlp commented Nov 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

tisonkun commented Nov 25, 2025 •

edited

Loading

orlp left a comment •

edited by rustbot

Loading

tisonkun Nov 26, 2025 •

edited

Loading

orlp commented Nov 26, 2025 •

edited

Loading